Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Electron Tau cross-trigger to the HLT Phase-2 menu #46924

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rdewanje
Copy link
Contributor

PR description:

Added the Electron+Tau cross trigger path to the existing Phase-2 HLT Menu after following the recipe given here:
https://cmshltupgrade.docs.cern.ch/RunningInstructions/

PR validation:

PR passed all test mentioned here:
https://cms-sw.github.io/PRWorkflow.html

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 12, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @rdewanje for master.

It involves the following packages:

  • HLTrigger/Configuration (hlt)

@Martin-Grunewald, @cmsbuild, @mmusich can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @SohamBhattacharya, @VourMa, @missirol, @mmusich, @rovere, @silviodonato this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

Copy link
Contributor

@mmusich mmusich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the PR title to be meaningful (as in "Add Electron Tau cross-trigger to the HLT Phase-2 menu")

from ..sequences.HLTPFTauHPS_cfi import *
from ..sequences.HLTHPSDeepTauPFTauSequence_cfi import *

#from ..sequences.RawToDigiSequence_cfi import * ## CHANGED TO FILE BELOW
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#from ..sequences.RawToDigiSequence_cfi import * ## CHANGED TO FILE BELOW

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

#from ..sequences.RawToDigiSequence_cfi import * ## CHANGED TO FILE BELOW
from ..sequences.HLTRawToDigiSequence_cfi import *

#from ..sequences.hgcalLocalRecoSequence_cfi import * ## CHANGED TO FILE BELOW
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#from ..sequences.hgcalLocalRecoSequence_cfi import * ## CHANGED TO FILE BELOW

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

#from ..sequences.hgcalLocalRecoSequence_cfi import * ## CHANGED TO FILE BELOW
from ..sequences.HLTHgcalLocalRecoSequence_cfi import *

#from ..sequences.localrecoSequence_cfi import * ## CHANGED TO FILE BELOW
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#from ..sequences.localrecoSequence_cfi import * ## CHANGED TO FILE BELOW

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 63 to 73



#hltEG30EtL1SeededFilter = _hltEG32EtL1SeededFilter.clone(
# etcutEB = cms.double(30.0),
# etcutEE = cms.double(30.0),
# inputTag = cms.InputTag("hltEgammaCandidatesWrapperL1Seeded"),
# l1EGCand = cms.InputTag("hltEgammaCandidatesL1Seeded"),
# ncandcut = cms.int32(1),
# saveTags = cms.bool(True)
#)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#hltEG30EtL1SeededFilter = _hltEG32EtL1SeededFilter.clone(
# etcutEB = cms.double(30.0),
# etcutEE = cms.double(30.0),
# inputTag = cms.InputTag("hltEgammaCandidatesWrapperL1Seeded"),
# l1EGCand = cms.InputTag("hltEgammaCandidatesL1Seeded"),
# ncandcut = cms.int32(1),
# saveTags = cms.bool(True)
#)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

HLTBeginSequence +
hltPuppiTauTkIsoEle4522L1TkFilter +

#RawToDigiSequence +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#RawToDigiSequence +

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

#RawToDigiSequence +
HLTRawToDigiSequence +

#hgcalLocalRecoSequence +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#hgcalLocalRecoSequence +

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

#hgcalLocalRecoSequence +
HLTHgcalLocalRecoSequence +

#localrecoSequence +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#localrecoSequence +

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -404,7 +405,7 @@
fragment.HLT_DoubleMediumChargedIsoPFTauHPS40_eta2p1,
fragment.HLT_DoubleMediumDeepTauPFTauHPS35_eta2p1,
fragment.HLT_IsoMu20_eta2p1_LooseDeepTauPFTauHPS27_eta2p1_CrossL1,

fragment.HLT_Ele30_WPTight_L1Seeded_LooseDeepTauPFTauHPS30_eta2p1_CrossL1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add this trigger also in the timing variant of the menu at HLTrigger/Configuration/python/HLT_75e33_timing_cff.py

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added this E-Tau path to the HLT_75e33_timing_cff.py file

@mmusich
Copy link
Contributor

mmusich commented Dec 12, 2024

test parameters:

  • enable = hlt_p2_integration
  • enable = hlt_p2_timing

@mmusich
Copy link
Contributor

mmusich commented Dec 13, 2024

@rdewanje please clarify if there will be follow-ups to the review. In particular addressing #46924 (comment) is necessary before starting tests.

@rovere
Copy link
Contributor

rovere commented Dec 13, 2024

Dear all, unless this new cross trigger path is added also to the timing flavour of the Phase-2 HLT menu, the timing measurements will be irrelevant/useless.

@mmusich
Copy link
Contributor

mmusich commented Dec 13, 2024

Dear all, unless this new cross trigger path is added also to the trigger flavour of the Phase-2 HLT menu, the timing measurements will be irrelevant/useless.

that's precisely my comment at #46924 (comment) and #46924 (comment) @rdewanje

@rovere
Copy link
Contributor

rovere commented Dec 13, 2024

Dear all, unless this new cross trigger path is added also to the trigger flavour of the Phase-2 HLT menu, the timing measurements will be irrelevant/useless.

that's precisely my comment at #46924 (comment) and #46924 (comment) @rdewanje

thanks Marco! Repetita iuvant. Sorry for the noise.

from ..modules.hltAK4PFJetsForTaus_cfi import *
from ..modules.hltHpsSelectedPFTauLooseTauWPDeepTau_cfi import *
from ..modules.hltHpsPFTau30LooseTauWPDeepTau_cfi import * ## NEW (with threshold different from youngwan)
from ..modules.hltAK4PFJets_cfi import *
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the module imported from this line is never used. I'd suggest to verify if that's correct and, if so, remove the import.

Copy link
Contributor Author

@rdewanje rdewanje Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, the line "from hltAK4PFJets_cfi import *" has been removed from the cfi file for the E-Tau path

@@ -0,0 +1,5 @@
import FWCore.ParameterSet.Config as cms

hltPuppiTauTkIsoEle4522L1TkFilter = cms.EDFilter("PathStatusFilter",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to change the name of the module and of the files by adding an underscore between 45 and 22.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rdewanje now this file is completely superseded by HLTrigger/Configuration/python/HLT_75e33/modules/hltPuppiTauTkIsoEle45_22L1TkFilter_cfi.py.
Please delete it and squash the commits in order to avoid having "virtual" files in the git history.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@rdewanje rdewanje changed the title Etau trigger pr branch Add Electron Tau cross-trigger to the HLT Phase-2 menu Dec 16, 2024
@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Pull request #46924 was updated. @Martin-Grunewald, @cmsbuild, @mmusich can you please check and sign again.

@rdewanje
Copy link
Contributor Author

Please update the PR title to be meaningful (as in "Add Electron Tau cross-trigger to the HLT Phase-2 menu")

Done.

@rdewanje
Copy link
Contributor Author

@rdewanje please clarify if there will be follow-ups to the review. In particular addressing #46924 (comment) is necessary before starting tests.

All comments regarding the PR have been addressed.

@cmsbuild
Copy link
Contributor

+1

Size: This PR adds an extra 48KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cde003/43470/summary.html
COMMIT: 4bcc213
CMSSW: CMSSW_15_0_X_2024-12-15-2300/el8_amd64_gcc12
Additional Tests: HLT_P2_TIMING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/46924/43470/install.sh to create a dev area with all the needed externals and cmssw changes.

  • DAS Queries: The DAS query tests failed, see the summary page for details.

  • HLT P2 Timing: chart

Comparison Summary

Summary:

  • You potentially removed 2 lines from the logs
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 18 differences found in the comparisons
  • DQMHistoTests: Total files compared: 46
  • DQMHistoTests: Total histograms compared: 3510017
  • DQMHistoTests: Total failures: 489
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3509508
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 45 files compared)
  • Checked 202 log files, 172 edm output root files, 46 DQM output files
  • TriggerResults: found differences in 1 / 44 workflows

from ..modules.hltHpsPFTau30LooseTauWPDeepTau_cfi import *


hltEle30WPTightClusterShapeL1SeededFilter = _hltEle32WPTightClusterShapeL1SeededFilter.clone(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for this module and all of the others below, I don't think that cloning an existing piece of configuration and then modifying a parameter from within the path itself is something we did elsewhere in the HLT menu.
I would actually create a new cfi file in HLTrigger/Configuration/python/HLT_75e33/modules

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But these additional files will cost additional memory. Besides it was recommended here: https://twiki.cern.ch/twiki/bin/view/CMSPublic/WorkBookConfigFileIntro#ClonIng

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But these additional files will cost additional memory.

I am not sure to see the point. Why would this cost additional memory?

Besides it was recommended here: https://twiki.cern.ch/twiki/bin/view/CMSPublic/WorkBookConfigFileIntro#ClonIng

while this is fine for general purposes, it's not the structure we decided to adopt for the phase-2 HLT menu. Please comply.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rdewanje please clarify if you are going to follow-up to #46924 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46924/43051

  • Found files with invalid states:
    • HLTrigger/Configuration/python/HLT_75e33/modules/hltPuppiTauTkIsoEle4522L1TkFilter_cfi.py:

@cmsbuild
Copy link
Contributor

Pull request #46924 was updated. @Martin-Grunewald, @cmsbuild, @mmusich can you please check and sign again.

@rdewanje
Copy link
Contributor Author

rdewanje commented Dec 19, 2024 via email

…r/Configuration/python/HLT_75e33/modules directory as requested
@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46924/43138

  • Found files with invalid states:
    • HLTrigger/Configuration/python/HLT_75e33/modules/hltPuppiTauTkIsoEle4522L1TkFilter_cfi.py:

@cmsbuild
Copy link
Contributor

Pull request #46924 was updated. @Martin-Grunewald, @cmsbuild, @mmusich can you please check and sign again.

@mmusich
Copy link
Contributor

mmusich commented Dec 22, 2024

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

+1

Size: This PR adds an extra 52KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cde003/43597/summary.html
COMMIT: 2cdafcc
CMSSW: CMSSW_15_0_X_2024-12-22-0000/el8_amd64_gcc12
Additional Tests: HLT_P2_TIMING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/46924/43597/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 1 lines to the logs
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 12 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3818730
  • DQMHistoTests: Total failures: 444
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3818266
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 214 log files, 184 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants